Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cmd-push-container-manifest: change image key schema #3129

Merged
merged 4 commits into from
Oct 21, 2022
Merged

cmd-push-container-manifest: change image key schema #3129

merged 4 commits into from
Oct 21, 2022

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Oct 20, 2022

The oscontainer and base-oscontainer keys should follow the same
schema. Currently, the former has a digest field, while the other one
does not. Tweak cosa push-container-manifest and cosa push-container
so that they follows the new schema. (Though note the latter command
will be deleted soon).

To keep previous 4.12 meta.json files valid, this loosens the image
schema definition so that digest is now optional. Once we branch for
4.12, we will undo this change so that it becomes required again.

Fixes #3122

@jlebon jlebon mentioned this pull request Oct 20, 2022
Merged
@jlebon jlebon force-pushed the pr/copy-container-prep branch from a84bfc7 to 1fdf599 Compare October 20, 2022 17:35
@jlebon jlebon changed the title cmd-push-container-manifest: change image format cmd-push-container-manifest: change image key schema Oct 20, 2022
@dustymabe
Copy link
Member

To keep previous 4.12 meta.json files valid, this loosens the image
schema definition so that digest is now optional. Once we branch for
4.12, we will undo this change so that it becomes required again.

So existing 4.12 meta.json files have base-oscontainer with an entry with image: repo@digest format and we're loosening the definition here so new cosa can continue to read those old meta.json files?

cgwalters
cgwalters previously approved these changes Oct 20, 2022
View reviewed changes
@jlebon jlebon force-pushed the pr/copy-container-prep branch from 1fdf599 to 9a958d8 Compare October 20, 2022 18:41
@jlebon
Copy link
Member Author

jlebon commented Oct 20, 2022

To keep previous 4.12 meta.json files valid, this loosens the image
schema definition so that digest is now optional. Once we branch for
4.12, we will undo this change so that it becomes required again.

So existing 4.12 meta.json files have base-oscontainer with an entry with image: repo@digest format and we're loosening the definition here so new cosa can continue to read those old meta.json files?

Exactly, yes. So one alternatives is to add a separate definition temporarily with the looser requirement, and then merge them back in later. I can do that if we want, but it didn't seem worth the overhead.

cgwalters
cgwalters previously approved these changes Oct 20, 2022
View reviewed changes
@jlebon jlebon mentioned this pull request Oct 20, 2022
Closed
@jlebon
Copy link
Member Author

jlebon commented Oct 20, 2022

Let's hold this until we discuss #3122 (comment).

@jlebon jlebon marked this pull request as draft October 20, 2022 21:14
@jlebon jlebon force-pushed the pr/copy-container-prep branch from 9a958d8 to c573a4b Compare October 21, 2022 13:42
@jlebon jlebon marked this pull request as ready for review October 21, 2022 13:42
@jlebon jlebon force-pushed the pr/copy-container-prep branch from c573a4b to 9addd2c Compare October 21, 2022 13:47
@jlebon jlebon mentioned this pull request Oct 21, 2022
Merged
@jlebon
Copy link
Member Author

jlebon commented Oct 21, 2022

Prow hitting

Wrote commit: 7f5db77028222497372a53f6a0f6f767331d59a5b91826d489df581c03e4f815
�[0m�[31merror: �[0mUpdating '/tmp/tmp.gKUD0uSyFg/tmp/treecompose.changed': Cannot allocate memory
error: failed to execute cmd-build: exit status 1

which looks like openshift/os#594, though that path doesn't look like a 9p mount.

/retest

dustymabe
dustymabe requested changes Oct 21, 2022
View reviewed changes
src/cosalib/container_manifest.py Show resolved Hide resolved
src/cosalib/container_manifest.py Outdated Show resolved Hide resolved
src/cmd-push-container-manifest Outdated Show resolved Hide resolved
src/cmd-push-container-manifest Outdated Show resolved Hide resolved
src/cosalib/container_manifest.py Outdated Show resolved Hide resolved
jlebon added 2 commits October 21, 2022 13:10
@jlebon
cmd-push-container-manifest: add support for multiple --tag arguments
9ae9b0e 
In the RHCOS pipeline, we want to be able to tag the same image using
both the stream name and the build ID.
@jlebon
cmd-push-container-manifest: change image key schema
e9b3b89 
The `oscontainer` and `base-oscontainer` keys should follow the same
schema. Currently, the former has a `digest` field, while the other one
does not. Tweak `cosa push-container-manifest` and `cosa push-container`
so that they follows the new schema. (Though note the latter command
will be deleted soon).

To keep previous 4.12 `meta.json` files valid, this loosens the `image`
schema definition so that `digest` is now optional. Once we branch for
4.12, we will undo this change so that it becomes required again.

Fixes coreos#3122
@jlebon jlebon force-pushed the pr/copy-container-prep branch from 9addd2c to 7441106 Compare October 21, 2022 17:25
@jlebon
Copy link
Member Author

jlebon commented Oct 21, 2022

Updated!

dustymabe
dustymabe reviewed Oct 21, 2022
View reviewed changes
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. A few more suggestions!

src/cosalib/container_manifest.py Outdated Show resolved Hide resolved
src/cmd-push-container-manifest Outdated Show resolved Hide resolved
dustymabe
dustymabe previously approved these changes Oct 21, 2022
View reviewed changes
@jlebon jlebon force-pushed the pr/copy-container-prep branch from 7441106 to 9fad3cb Compare October 21, 2022 17:55
@jlebon
cmd-push-container-manifest: store arch-specific digest in meta.json
d40984d 
As discussed in coreos#3122,
ART needs the arch-specific digest in the `meta.json` rather than the
manifest list digest. FCOS isn't planning to use the digest so doesn't
care about which one gets chosen.

Update `cosa push-container-manifest` to use the arch-specific digest.
@jlebon jlebon force-pushed the pr/copy-container-prep branch from 9fad3cb to d40984d Compare October 21, 2022 17:57
dustymabe
dustymabe approved these changes Oct 21, 2022
View reviewed changes
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jlebon jlebon enabled auto-merge (rebase) October 21, 2022 18:19
@jlebon
Copy link
Member Author

jlebon commented Oct 21, 2022

/override ci/prow/rhcos

It doesn't test this and it's taking too long. It passed on other derivative PRs that include almost exactly this code anyway.

@openshift-ci
Copy link

openshift-ci bot commented Oct 21, 2022

@jlebon: Overrode contexts on behalf of jlebon: ci/prow/rhcos

In response to this:

/override ci/prow/rhcos

It doesn't test this and it's taking too long. It passed on other derivative PRs that include almost exactly this code anyway.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jlebon jlebon merged commit c7b5757 into coreos:main Oct 21, 2022
@jlebon jlebon mentioned this pull request Oct 24, 2022
Open
6 tasks
@jlebon jlebon deleted the pr/copy-container-prep branch April 22, 2023 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dustymabe dustymabe dustymabe approved these changes

@cgwalters cgwalters cgwalters left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

formatting of "oscontainer" entries in the schema
4 participants
@jlebon @dustymabe @cgwalters @openshift-merge-robot